Skip to content

3.13 test #585

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Jul 28, 2025
Merged

3.13 test #585

merged 3 commits into from
Jul 28, 2025
+7 −15

Conversation

@misrasaurabh1
Copy link
Contributor

@misrasaurabh1 misrasaurabh1 commented Jul 25, 2025 •
edited by github-actions bot
Loading

PR Type

Tests, Enhancement

Description

  • Normalize test f-string quoting conditions

  • Update assertions replacing double quotes

  • Add Python 3.13 to CI testing matrix

Diagram Walkthrough

flowchart LR
  PR["Pull Request"] -->|"Modify test assertions"| Tests["tests/test_instrument_tests.py"]
  PR -->|"Update CI matrix"| CI[".github/workflows/unit-tests.yaml"]
Loading

File Walkthrough
Relevant files
Tests
test_instrument_tests.py
Unify test string formatting and comparisons                         

tests/test_instrument_tests.py

  • Removed version-specific f-string quoting conditional
  • Unified expected test_stdout_tag f-string syntax
  • Updated assertions to replace double quotes with single
 +6/-14   
Configuration changes
unit-tests.yaml
Add Python 3.13 to CI matrix                                                         

.github/workflows/unit-tests.yaml

  • Added Python 3.13 to CI testing matrix
  • Extended CI job strategy matrix
 +1/-1     

@misrasaurabh1
3.13 test
3a44a44 
Signed-off-by: Saurabh Misra <[email protected]>
@github-actions github-actions bot added workflow-modified This PR modifies GitHub Actions workflows Review effort 1/5 labels Jul 25, 2025
@github-actions
Copy link

github-actions bot commented Jul 25, 2025 •
edited
Loading

PR Reviewer Guide 🔍

(Review updated until commit 8ba7827)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Quotation Normalization

Using replace('"', "'") on both actual and expected outputs may mask other discrepancies and make tests less precise. Consider a more targeted normalization approach.

 
assert new_test.replace('"', "'") == expected.format(
    module_path=Path(f.name).name, tmp_dir_path=get_run_tmp_file(Path("test_return_values"))
).replace('"', "'")
Newline in Triple-Quoted Strings

The triple-quoted blocks for expected strings might introduce unintended newlines or whitespace. Verify that the expected strings exactly match actual output formatting.

 
"""
expected += """test_stdout_tag = f'{{test_module_name}}:{{(test_class_name + '.' if test_class_name else '')}}{{test_name}}:{{function_name}}:{{loop_index}}:{{invocation_id}}'
"""
CI Matrix Version

Ensure GitHub Actions supports Python 3.13 in the setup-python action and that the matrix syntax remains valid.

 
python-version: ["3.9", "3.10", "3.11", "3.12", "3.13"]

@github-actions
Copy link

github-actions bot commented Jul 25, 2025 •
edited
Loading

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Match quotes to remove replace

Use double quotes inside the f-string literal in expected so it matches the
generated code and eliminates the need for global .replace('"', "'") normalization.
This makes the assertion more direct and less error-prone.

tests/test_instrument_tests.py [132-133]

-expected += """test_stdout_tag = f'{{test_module_name}}:{{(test_class_name + '.' if test_class_name else '')}}{{test_name}}:{{function_name}}:{{loop_index}}:{{invocation_id}}'
+expected += """test_stdout_tag = f"{test_module_name}:{(test_class_name + '.' if test_class_name else '')}{test_name}:{function_name}:{loop_index}:{invocation_id}"
 """
Suggestion importance[1-10]: 5

__

Why: Changing the f-string to use double quotes directly aligns expected with generated output and reduces need for manual replace without altering behavior.

Low
Extract normalization before assert

Normalize both new_test and the formatted expected into intermediate variables
before asserting equality. This improves readability and makes the transformation
intent clear.

tests/test_instrument_tests.py [188-190]

-assert new_test.replace('"', "'") == expected.format(
+normalized_new = new_test.replace('"', "'")
+normalized_expected = expected.format(
     module_path=Path(f.name).name, tmp_dir_path=get_run_tmp_file(Path("test_return_values"))
 ).replace('"', "'")
+assert normalized_new == normalized_expected
Suggestion importance[1-10]: 4

__

Why: Introducing intermediate normalized_new and normalized_expected vars clarifies the normalization step and enhances readability, but has minimal functional impact.

Low

@misrasaurabh1 misrasaurabh1 requested a review from KRRT7 July 25, 2025 21:31
@misrasaurabh1 misrasaurabh1 marked this pull request as ready for review July 25, 2025 21:31
@github-actions
Copy link

Persistent review updated to latest commit 8ba7827

@github-actions
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Match quotes to remove replace

Use double quotes inside the f-string literal in expected so it matches the
generated code and eliminates the need for global .replace('"', "'") normalization.
This makes the assertion more direct and less error-prone.

tests/test_instrument_tests.py [132-133]

-expected += """test_stdout_tag = f'{{test_module_name}}:{{(test_class_name + '.' if test_class_name else '')}}{{test_name}}:{{function_name}}:{{loop_index}}:{{invocation_id}}'
+expected += """test_stdout_tag = f"{test_module_name}:{(test_class_name + '.' if test_class_name else '')}{test_name}:{function_name}:{loop_index}:{invocation_id}"
 """
Suggestion importance[1-10]: 5

__

Why: Changing the f-string to use double quotes directly aligns expected with generated output and reduces need for manual replace without altering behavior.

Low
Extract normalization before assert

Normalize both new_test and the formatted expected into intermediate variables
before asserting equality. This improves readability and makes the transformation
intent clear.

tests/test_instrument_tests.py [188-190]

-assert new_test.replace('"', "'") == expected.format(
+normalized_new = new_test.replace('"', "'")
+normalized_expected = expected.format(
     module_path=Path(f.name).name, tmp_dir_path=get_run_tmp_file(Path("test_return_values"))
 ).replace('"', "'")
+assert normalized_new == normalized_expected
Suggestion importance[1-10]: 4

__

Why: Introducing intermediate normalized_new and normalized_expected vars clarifies the normalization step and enhances readability, but has minimal functional impact.

Low

@misrasaurabh1 misrasaurabh1 merged commit 52083c4 into main Jul 28, 2025
19 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@KRRT7 KRRT7 KRRT7 approved these changes

Assignees

No one assigned

Labels

Review effort 2/5 workflow-modified This PR modifies GitHub Actions workflows

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@misrasaurabh1 @KRRT7